Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add query comment into the query for improved debugging #1082

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Add query comment into the query for improved debugging #1082

wants to merge 2 commits into from

Conversation

wwoytenko
Copy link

Closes #998

A common challenge when using ORMs is the difficulty of correlating an executed query in the DBMS with the lines of code that triggered it. To solve this issue, it would be helpful to add a comment in the header of the generated query. In this implementation, if a context with a comment is provided, we’ll include that as a comment in the query.

This feature enhances debugging by including query comments in headers, allowing developers to trace the query's origin. By matching the query comment in the header, it's easier to identify the specific location in the code where the query was executed.

Using this feature uses now can add SQL comment in query header

For example

db.NewUpdate().
   Model(&Model{}).
   Comment("test").
   Set("name = ?", "new-name").
   Where("id = ?", 1)

This will create a query like

/* test */ UPDATE "models" AS "model" SET name = 'new-name' WHERE (id = 1)

This was implemented in go-pg #2011

@wwoytenko wwoytenko force-pushed the feat/add_query_comment branch 2 times, most recently from b5a3236 to 17bb978 Compare December 3, 2024 16:47
@wwoytenko
Copy link
Author

Looks like it is similar to #1002.

@wwoytenko wwoytenko force-pushed the feat/add_query_comment branch from 17bb978 to b1e4e60 Compare December 3, 2024 16:55
@wwoytenko wwoytenko force-pushed the feat/add_query_comment branch from b1e4e60 to 1376d18 Compare December 3, 2024 17:05
Aoang
Aoang suggested changes Dec 9, 2024
View reviewed changes
util.go
@@ -66,3 +70,13 @@ func sliceElemType(v reflect.Value) reflect.Type {
}
return indirectType(elemType)
}

// appendComment adds comment in the header of the query into buffer
func appendComment(b []byte, name string) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here need to filter 0x00

appendComment(nil, string([]byte{'*', 0, 0, 0, 0, 0, '/'}))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Added fix + test

@wwoytenko wwoytenko requested a review from Aoang December 10, 2024 07:02
@vmihailenco
Copy link
Member

Let's move comment string to baseQuery with accompanying method to add/set a comment that is called from Select/InsertQuery.

Also, do we want to support a single comment or multiple comments? E.g. what should be the result of calling q.Comment("comment1").Comment("comment2"). I'd say that it is more natural to allow multiple comments, but if we don't know the answer yet, let's forbid the 2nd Comment call by setting an error on the query.

@wwoytenko
Copy link
Author

@vmihailenco I can add multicomment support.
Current implementation add comment with space seapration

/* com */ select 1;

But I wonder should they be separeated by the new line line

/* com1 */
/* com2 */
select 1;

I suspect there might be a case when we have sharded db and we need to add kind of global instance name in the comment for example when we establishing a connection or via some hook. In this particular case it might be:

/* shard1 */
/* query commment */
select 1;

What do you think?

@Aoang
Copy link
Contributor

Aoang commented Dec 17, 2024

It is not advisable to implement support for multiple comments now. When writing SQL using an ORM, the syntax is relatively unordered; however, comments should maintain a specific sequence.
Introducing support for multiple comments now could hinder our ability to implement ordered comments in the future, even if we are currently confident that such an implementation will not take place.

For example:

/* maybe we need to write something */
SELECT /* but seem redundant */ 1;
/* say goodbye to db */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aoang Aoang Awaiting requested review from Aoang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Is it possible to insert a comment before or after a sql query?
3 participants
@wwoytenko @vmihailenco @Aoang